Skip to content

Govern sharing of restricted data by observer verification - #340

Closed
Maximo-Guk wants to merge 14 commits into
mainfrom
restricted-data
Closed

Govern sharing of restricted data by observer verification#340
Maximo-Guk wants to merge 14 commits into
mainfrom
restricted-data

Conversation

@Maximo-Guk

@Maximo-Guk Maximo-Guk commented Aug 25, 2026

Copy link
Copy Markdown
Member

Why:

Before this PR: Once a user has observed a sensitive data source, we block sharing the resource altogether.

We decided we can lift this restriction now that we have mechanisms in place to verify the observers permissions.

What:

This PR sets out to rename prohibitAllSharing to containsRestrictedData and makes it so that now when restricted data is observed, the workspace can still be shared by verifying the observers access against the resource by reusing many of the same mechanisms we had already introduced in the original observer verification PR.

authorizeCollaborator() is now responsible for being the single gate for checking whether observers have access to underyling data. Everything routes through here, including receiveExternalMessage() which previously only checked the role and never checked whether the observer had access to the underlying data.

We also decided to abort the DO in order to revoke the RPC capabilities, we already use the same mechanism when revoking collaborators and we can just the same mechanism here. We made this a little less disruptive by checking graph first to see if there's any other collaborators before aborting the DO

Testing:

I added extensive integration tests in sensitive-observations.test.ts, observer-role-scope.test.ts and external-message-verification ( even though it's actually used yet in any gatekeepers ) for testing the observer verification logic

I also have two preview links mantainers can test it out on

Please note these previews are built off of #308 (which is stacked on this PR) since this PR contains no UI changes

Out of scope:

There were a few pre-existing issues which my agent flagged, I left/flagged them as TODO's in the PR.

Frontend behavior is unchanged here, the share modal edits are the rename only, so the UI still declines to share a workspace even though the server now permits it. The UI will be done in #308

@github-actions github-actions Bot added workshop/frontend Changes to the Workshop frontend kernel Changes to the Workshop kernel gatekeeper Changes to a gatekeeper integration workshop/shared Changes to shared Workshop APIs labels Aug 25, 2026

@devin-ai-integration devin-ai-integration Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Devin Review: No Issues Found

Devin Review analyzed this PR and found no potential bugs to report.

View in Devin Review to see 1 additional finding.

Open in Devin Review

@github-actions

Copy link
Copy Markdown

Preview: pr340-restricted-data

https://pr340-restricted-data-router.cloudflare-os-previews.workers.dev

Dashboard · deleted when this PR closes

@Maximo-Guk
Maximo-Guk force-pushed the restricted-data branch 10 times, most recently from 38b7ea6 to db4644b Compare August 27, 2026 16:38
Maximo-Guk added a commit that referenced this pull request Aug 27, 2026
Comments and the plan doc still described two-phase redemption: a failed
open reverting the redemption server-side, so a retry had to re-send the
key, and success "confirming" it. Under one-step redemption (#340) the
edge is real the moment the server redeems, so a failure after that
point retries keylessly and nothing is ever reverted or confirmed.

The mechanism is unchanged and still earns its keep: an open can fail
*before* the redemption lands (a transport failure, a server throw ahead
of the redemption, an attempt superseded before issuing), and the client
cannot distinguish that from a post-redemption failure, so it retains
the key on every failure -- replaying a key whose edge already exists is
a server-side no-op. Prose only; no behavior change.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@Maximo-Guk
Maximo-Guk marked this pull request as ready for review August 27, 2026 18:17
Maximo-Guk added a commit that referenced this pull request Aug 27, 2026
Comments and the plan doc still described two-phase redemption: a failed
open reverting the redemption server-side, so a retry had to re-send the
key, and success "confirming" it. Under one-step redemption (#340) the
edge is real the moment the server redeems, so a failure after that
point retries keylessly and nothing is ever reverted or confirmed.

The mechanism is unchanged and still earns its keep: an open can fail
*before* the redemption lands (a transport failure, a server throw ahead
of the redemption, an attempt superseded before issuing), and the client
cannot distinguish that from a post-redemption failure, so it retains
the key on every failure -- replaying a key whose edge already exists is
a server-side no-op. Prose only; no behavior change.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Maximo-Guk added a commit that referenced this pull request Aug 27, 2026
Comments and the plan doc still described two-phase redemption: a failed
open reverting the redemption server-side, so a retry had to re-send the
key, and success "confirming" it. Under one-step redemption (#340) the
edge is real the moment the server redeems, so a failure after that
point retries keylessly and nothing is ever reverted or confirmed.

The mechanism is unchanged and still earns its keep: an open can fail
*before* the redemption lands (a transport failure, a server throw ahead
of the redemption, an attempt superseded before issuing), and the client
cannot distinguish that from a post-redemption failure, so it retains
the key on every failure -- replaying a key whose edge already exists is
a server-side no-op. Prose only; no behavior change.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@Maximo-Guk
Maximo-Guk marked this pull request as draft August 27, 2026 22:53
Maximo-Guk added a commit that referenced this pull request Aug 28, 2026
Comments and the plan doc still described two-phase redemption: a failed
open reverting the redemption server-side, so a retry had to re-send the
key, and success "confirming" it. Under one-step redemption (#340) the
edge is real the moment the server redeems, so a failure after that
point retries keylessly and nothing is ever reverted or confirmed.

The mechanism is unchanged and still earns its keep: an open can fail
*before* the redemption lands (a transport failure, a server throw ahead
of the redemption, an attempt superseded before issuing), and the client
cannot distinguish that from a post-redemption failure, so it retains
the key on every failure -- replaying a key whose edge already exists is
a server-side no-op. Prose only; no behavior change.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@Maximo-Guk

Copy link
Copy Markdown
Member Author

Rebased off main

@ask-bonk

ask-bonk Bot commented Aug 28, 2026

Copy link
Copy Markdown

Findings

  1. P1 overseer.ts:4527,7987,11404: Restricted data from unbound producers can reach gadget state/UI, but use collaborators are never verified against those producers. Verify all restricted producers or enforce provenance isolation.

  2. P1 gatekeeper-google/src/google.ts:983,3399: BigQuery verification only calls datasets.get. This does not prove table-data access or equivalent row/column policy results, allowing collaborators to see owner-only query output.

  3. P1 overseer.ts:4388-4407: addGatekeeper() publishes the connection before awaiting describe(), but restarts collaborators afterward. Existing build sessions can access the predictable ID during this unbounded window without verification.

  4. P1 overseer.ts:8200,8319-8328: Failed re-verification scrubs coverage, then may wait indefinitely for the configuration callback and cleanup before scheduling restart. Previously opened sessions remain usable after revocation is detected.

github run

@Maximo-Guk

Maximo-Guk commented Aug 28, 2026

Copy link
Copy Markdown
Member Author

Findings

  1. P1 overseer.ts:4527,7987,11404: Restricted data from unbound producers can reach gadget state/UI, but use collaborators are never verified against those producers. Verify all restricted producers or enforce provenance isolation.
  2. P1 gatekeeper-google/src/google.ts:983,3399: BigQuery verification only calls datasets.get. This does not prove table-data access or equivalent row/column policy results, allowing collaborators to see owner-only query output.
  3. P1 overseer.ts:4388-4407: addGatekeeper() publishes the connection before awaiting describe(), but restarts collaborators afterward. Existing build sessions can access the predictable ID during this unbounded window without verification.
  4. P1 overseer.ts:8200,8319-8328: Failed re-verification scrubs coverage, then may wait indefinitely for the configuration callback and cleanup before scheduling restart. Previously opened sessions remain usable after revocation is detected.

github run

From scanning these findings with my agent:

  1. Already documented as "Known security risk — never-bound producers"

  2. This is addressed in Extract the BigQuery observer probe out of google.ts #372 , we're only touching kernel changes here

  3. This should be fixed in my last commit

  4. The re-prompt stall is equivalent to never re-opening (edge case 3)

@ask-bonk

ask-bonk Bot commented Aug 28, 2026

Copy link
Copy Markdown

Findings

  1. P1 gatekeeper.ts:1094, overseer.ts:4523, google.ts:3399: The hard flag rename is unsafe across independently deployed workers. Mixed versions silently ignore the optional unknown property, allowing restricted BigQuery observations without latching. Temporarily dual-read and dual-write both names.

  2. P1 overseer.ts:2245-2256: A permanent binding is exposed before the asynchronous restart. An existing use session can invoke gadget code through the newly bound connection without observer verification during this window. Invalidate stale sessions before publishing the edge.

  3. P2 overseer.ts:4519: Every observation now resolves SharingManager, including unrestricted hook observations. On a cold DO this requires an owner User DO RPC, so unrelated transient failures block ordinary observations. Fetch it only inside the restricted-data branch.

  4. P2 overseer.ts:3698,3717,3810: Every promoted gadget or binding triggers a workspace restart even when effective observer scope is unchanged, such as an empty gadget or vendorless binding. Compare scope before and after promotion.

github run

// Action records that predate the flag's rename carry `containsRestrictedData` under its old
// name, `prohibitAllSharing`. Records are data at rest and are never rewritten, so the
// tolerance can never be removed.
type LegacyObservationDescription = ObservationDescription & { prohibitAllSharing?: boolean };

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is pretty ugly. We should either do a migration to update all existing records, or we should continue using the old name in the meantime. (There's other precedent for continuing to use old names in the storage schemas already. I am sort of inclined to extend typed-storage to support specifying a "legacy name" for a collection/singleton/index/etc., in order to avoid migrations...)

Maximo-Guk and others added 14 commits August 28, 2026 14:02
The persisted observer record is the standing claim that a collaborator was verified
for a producer -- `ensureObserver` reads it back on their next open and re-registers
them off the account choice it holds, and `authorizeObservation` reads it from other
turns. So a collaborator whose live re-verification just failed must not keep an entry
saying they are covered: until now a revoked collaborator stayed "verified" until their
next *successful* open.

`fail()` now drops the failed gatekeeper from the persisted `accountChoices`
synchronously with the failure determination, `getVerifier` moves inside the
per-gatekeeper `try` so a verifier-acquisition rejection scrubs like any other refusal
(and surfaces the descriptive denial rather than the raw RPC error, with no mid-flight
`Promise.all` rejection to stale the rollback snapshot), and the terminal catch
de-registers invalidated registrations alongside newly-added ones.

That last part is a fail-open regression for a *returning* observer, marked with a TODO
here and fixed in the next commit.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The persisted observer record is meant to state what a collaborator's most recent open
verified, and `ensureObserver` re-registers them off the account choices it holds. But
a choice for a gatekeeper outside their current verification scope survived every open
that could not check it: a "use" collaborator who opens while a connection is unbound
from every gadget verifies nothing against it, yet their stale entry stays -- and the
moment the connection is rebound (rebinding keeps the same gatekeeper id) the next open
silently re-registers them off a choice made for a scope the workspace no longer has,
instead of asking them again.

Step 2 now drops every account choice for a gatekeeper outside the collaborator's live
verification scope, even when the remaining scope is empty (that is exactly the
everything-unbound open). The gatekeeper-side registration is deliberately kept: it
preserves forward exclusion via `byObserverId`, and the next successful open's
`addObserver` overwrites the verifier.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…n fails.

`ensureObserver`'s rollback removed the gatekeeper registrations of every binding that
failed the call (`invalidated`), not just the ones the call created (`newlyAdded`). For a
collaborator who was already an admitted observer, that de-registered them from a
gatekeeper they had previously been verified against -- and a de-registered observer is
one the gatekeeper stops naming in `ObservationDescription.excludeObservers`, so an
observation it would have excluded them from is admitted with nothing left to block it.

The coverage scrub this rollback accompanies is not a substitute for the registration.
They cover different sets: `gatekeeper-confluence` -- the only in-repo producer of
`excludeObservers` -- never marks an observation `prohibitAllSharing`, so for it the
scrub covers none of the affected reads. The reachable sequence is a collaborator whose
Confluence access is revoked upstream, whose re-open therefore fails, and whose
pre-existing live session then watches the owner's agent read a page they cannot access.

So roll back `invalidated` only on a first-ever verification, where the minted
observerId is discarded along with the unpersisted record and a registration left behind
would linger unresolvable. A returning observer's id is already persisted, so keeping
their registrations is fail-closed (a registration can only add exclusion names) and the
next successful open's `addObserver` overwrites the verifier. This restores the invariant
`registeredBeforeCall` was introduced to state: roll back only what this call added.
Coverage is still scrubbed either way, so a revoked collaborator's record stops claiming
they were verified.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ens.

Authorization and observer verification run only at open(). Nothing re-ran
them when the set of gatekeepers a collaborator must be verified against
*grew* mid-session -- adding a connection, or binding one into a gadget --
so a collaborator who opened before the growth kept a live session holding
access they were never verified for.

Fix it with the mechanism already used to revoke a collaborator: generalize
scheduleRevocationRestart() to scheduleAccessRestart(reason) and add
#restartIfShared(), which flushes, waits 100ms and aborts the DO so every
client reconnects and re-opens against the new scope. It is a no-op when the
workspace has no collaborators, so a solo workspace is never disturbed.

Four sites widen scope and now restart: addGatekeeper, a permanent
bindWorkpiece, a merge that promotes a binding edge into "use" scope, and a
denied re-verification that scrubbed a persisted account choice. The merge
case compares the effective account-requiring "use" scope before and after
promotion rather than restarting on any promotion, since most merges promote
neither a gadget with bindings nor an edge to a connection anyone is verified
against. It reads the scope through the non-throwing gatekeeperVendorId()
rather than #inScopeGatekeepers("use"), whose observerVendorId() throws on a
legacy record with no creationSpec -- an unrelated legacy connection must not
turn an accepted merge into an error.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`addGatekeeper` published the gatekeeper record before awaiting the gatekeeper's
`describe()`, because `getGatekeeperFacet(id)` resolved the class from that record. The DO's
input gate is open across the await and ids are allocated sequentially, so a live `build`
session could guess the id and `getGatekeeperById()`/`openSession()` on the owner's
brand-new connection -- which gates on nothing but record existence -- for as long as
`describe()` took, all of it before `#restartIfShared` severed it.

`getGatekeeperFacet` now optionally takes the class directly, so the record is published
exactly once, after `describe()` resolves. Nothing a gatekeeper's `describe()` can reach
calls back into the overseer to resolve itself by record, so no caller needs the early put.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
receiveExternalMessage() checked only the caller's role. Observer
verification -- which is how a collaborator earns the right to see what the
workspace has read -- runs at open(), so a "build" collaborator who never
opened the workspace, or whose upstream access was since revoked, could still
drive the agent and have it answer out of chat history and gadget storage.

Extract the gate open() applies into authorizeCollaborator(): resolve the
effective role from the permission graph, then run ensureObserver for that
role. Both entry points call it. The external path passes requireRole:
"build", so an insufficient role is denied before verification runs -- a "use"
collaborator would otherwise be verified, or told to go fix a verification
failure, for access this path can never grant them. It also passes no
configureCb, since there is no channel to prompt on: an unverified caller is
told to open the workspace in a browser, which is where configuration happens.

roleRank is exported for the requireRole comparison, so it ranks rather than
string-compares.

Also has open() await ambient reconciliation before authorizing rather than
between the role check and verification, which is where the two halves now
join.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
docs/observers.md gains a "Restarting when verification scope widens"
subsection: the four triggers, why the merge trigger compares scopes rather
than firing on any promotion, why shrinking scope and role rises are
deliberately not triggers, why addGatekeeper's publication order is
load-bearing under the restart, and where the enforcement moment actually
falls for each trigger. Step 3 gains the record prune, the scrub-and-restart
failure path and the returning-observer rollback rule; edge cases 3 and 5 are
rewritten around them, and Step 6's justification for an orphaned entry is
corrected -- a registration is what admits an open, so a stale one grants
nothing on its own.

docs/sharing.md renames scheduleRevocationRestart and documents the abort's
second purpose, whose trigger is a grant rather than a revocation.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
A schema property name is also the KV key it maps to, so renaming a property
in code is a storage migration. Give a singleton slot somewhere to say
otherwise: `singleton(defaultValue, {storageKey})` declares the key on disk
explicitly, and a bare default value stays the shorthand for the common case
and behaves exactly as before. Collections get the same option as
`storageName`, which prefixes the records and every index alike.

This is the schema-level version of what would otherwise be a special case at
each call site, and it keeps the old name on disk with no migration.
The flag's real meaning is "this observation contains restricted data". What
the platform does about that is policy, which shouldn't be baked into the name
-- the next commits replace the all-or-nothing lockdown with per-collaborator
observer verification.

ObservationDescription.prohibitAllSharing and GadgetMetadata.sharingProhibited
both become containsRestrictedData. No alias: this is a hard rename, so the
gatekeeper call sites move in the same commit.

The overseer's durable singleton is renamed too, and declares its old name as
its `storageKey` so nothing on disk moves. Without that, every workspace that
has already observed restricted data would silently unlatch.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Restate what `ObservationDescription.containsRestrictedData` means now that the
enforcement is per-collaborator observer verification rather than an
all-or-nothing sharing lockdown, and state the two limits of the model plainly:
verification is held to the collaborator's role scope, and enforcement is at
admission rather than at each read.

No functional change; the implementation follows.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ion.

Reading restricted data no longer locks the workspace down. The old model
blocked the observation outright if the workspace was shared and then refused
all future sharing, which made every sensitive data source unusable the moment
a workspace had a single collaborator. The observer verification machinery
already answers the real question -- does this collaborator have access to the
same data? -- at every open, and a widening of that scope now restarts every
live session, so admission is a sound enforcement point.

So: drop the `hasAnyShares()` block in `authorizeObservation` and the three
guards on the sharing mutators. Keep the two guards that are about leaking data
back out rather than about who may see it -- no actions and no public web
fetches once the latch is set.

What replaces them is narrower. A producer nobody can ever be verified against
(a vendorless connection, or a legacy record with no `creationSpec`) is still
refused while the workspace is shared, because `#inScopeGatekeepers` skips it
and so admission cannot see it at all. Removing a producer's record is blocked
while the workspace is shared, since that record is what verification runs
against. And a new grant -- a collaborator, a share link, another key for one,
or a redemption -- is refused if some producer can no longer verify anyone.
Each of those checks runs in the same synchronous block as the write it gates,
after every await, so a concurrent change cannot slip between check and write.

`sharing.ts` loses `hasAnyShares()` and gains an optional `assertGrantAllowed`
on each grant-writing method, invoked at that write.

Two smaller things fall out. `getSharingManager()` moves inside the
`containsRestrictedData` branch, so an ordinary observation on a cold DO no
longer pays for an owner User DO round trip; the producer record is then read
after that await, since latching against a stale record would permanently brick
sharing. And the restart on a terminal re-verification failure is hoisted ahead
of the best-effort rollback, taking a gatekeeper RPC fan-out off the path
between determining the denial and the abort.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Covers the latch (what sets it, and the cases that must refuse the read rather
than latch), the producer-removal guard and its exemptions, the grant checks on
each sharing mutator, and the tolerance for action records written before the
flag's rename.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Drives the model end to end through the test gatekeeper: a restricted read on a
shared workspace, the unverifiable-producer refusal, the removal guard, the
action and web-fetch blocks, and the restart that forces re-verification when
scope widens.

`TestSession.readThing()` takes an optional `restricted` flag so a test can trip
the latch through the same `ApprovalQueue` funnel a shipping gatekeeper uses.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Rewrites the observer document's model section around admission-time
enforcement, states the two limits (role-scoped verification, and enforcement
at admission rather than at each read) as edge cases with their reasoning, and
records the design under plans/restricted-data-sharing.md -- including the
known risk of a producer no gadget binds.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@Maximo-Guk

Maximo-Guk commented Aug 28, 2026

Copy link
Copy Markdown
Member Author

Superseded by a three-way split, stacked in this order: #380

@Maximo-Guk Maximo-Guk closed this Aug 28, 2026
Maximo-Guk added a commit that referenced this pull request Aug 28, 2026
Comments and the plan doc still described two-phase redemption: a failed
open reverting the redemption server-side, so a retry had to re-send the
key, and success "confirming" it. Under one-step redemption (#340) the
edge is real the moment the server redeems, so a failure after that
point retries keylessly and nothing is ever reverted or confirmed.

The mechanism is unchanged and still earns its keep: an open can fail
*before* the redemption lands (a transport failure, a server throw ahead
of the redemption, an attempt superseded before issuing), and the client
cannot distinguish that from a post-redemption failure, so it retains
the key on every failure -- replaying a key whose edge already exists is
a server-side no-op. Prose only; no behavior change.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@Maximo-Guk
Maximo-Guk removed this pull request from stack #346 September 11, 2026 01:19
Maximo-Guk added a commit that referenced this pull request Sep 11, 2026
Comments and the plan doc still described two-phase redemption: a failed
open reverting the redemption server-side, so a retry had to re-send the
key, and success "confirming" it. Under one-step redemption (#340) the
edge is real the moment the server redeems, so a failure after that
point retries keylessly and nothing is ever reverted or confirmed.

The mechanism is unchanged and still earns its keep: an open can fail
*before* the redemption lands (a transport failure, a server throw ahead
of the redemption, an attempt superseded before issuing), and the client
cannot distinguish that from a post-redemption failure, so it retains
the key on every failure -- replaying a key whose edge already exists is
a server-side no-op. Prose only; no behavior change.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Maximo-Guk added a commit that referenced this pull request Sep 11, 2026
Comments and the plan doc still described two-phase redemption: a failed
open reverting the redemption server-side, so a retry had to re-send the
key, and success "confirming" it. Under one-step redemption (#340) the
edge is real the moment the server redeems, so a failure after that
point retries keylessly and nothing is ever reverted or confirmed.

The mechanism is unchanged and still earns its keep: an open can fail
*before* the redemption lands (a transport failure, a server throw ahead
of the redemption, an attempt superseded before issuing), and the client
cannot distinguish that from a post-redemption failure, so it retains
the key on every failure -- replaying a key whose edge already exists is
a server-side no-op. Prose only; no behavior change.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Maximo-Guk added a commit that referenced this pull request Sep 11, 2026
…es (#308)

* Restricted data part 5: UI changes.

The Share modal no longer replaces itself with a "can't be shared" view when
the workspace has read restricted data. Sharing controls stay live and a
notice explains that collaborators must be able to see the data themselves.

The server allows sharing after the restricted latch (assertNewSharingAllowed
refuses only unverifiable producers) and GadgetMetadata.containsRestrictedData
documents that such a workspace can still be shared, so the modal's job is to
warn and to surface a server refusal verbatim -- which the existing toast
catches already do. Regression tests pin both: with the flag set, the banner
renders in place of the wall and every management affordance (invite, link
creation/copying, collaborator removal, link revocation) stays reachable; and
a server-side "can no longer be shared" rejection reaches the user as an
error toast.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

* Retain a consumed share key so a failed open can retry.

Opening with a #share= fragment strips the key from the URL, so an open that
failed while the recipient's access was still being verified had nothing left
to retry with. The key is now held and replayed on the next attempt.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

* Bugfix: Identity-key retained share keys and clear them on logout.

The retained key moves to sessionStorage so a reload can still retry, which
means it can outlive the session that captured it. Each entry is therefore
stamped with the capturing user's id and ignored -- and swept -- when the
current session's id doesn't match, so one user's pending share key can never
be redeemed under the next user's account in the same tab.

logout() sweeps the whole prefix as well, including malformed and older
unstamped entries.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

* Bugfix: Abandon a superseded workspace open before it creates a capability.

The retained-storage path awaits identity resolution before openGadget. An
attempt superseded while parked there had already run its cleanup -- with
nothing yet to dispose -- so on resuming it minted a stub its cleanup can never
reach and published it over the replacement attempt's state: a stale
capability, or the wrong workspace's when the id changed.

Bail after the await, before any capability is created, like the checks the
later awaits already have.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

* Bugfix: Bind the retained share key to the session that captured it.

The in-memory retention tier carried no identity and was replayed on whatever
authenticated stub the effect ran with. Its safety rested on a rendering
invariant two files away -- that an identity change always unmounts the editor
-- which nothing local enforced; an account switcher or soft logout would have
silently turned it into a cross-user key replay.

The ref now records the stub that captured it and is replayed only on that
stub. Any other stub falls through to the sessionStorage tier, whose entries
are identity-stamped and checked. Stub identity rather than an async userId
keeps the common same-session retry pipelined.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

* Bugfix: Invalidate a pending share-key stamp on discard and logout.

The capture path's identity stamp is asynchronous, gated by a flag local to
one load attempt -- but the storage it writes is global. A stamp resolving
after a *different* attempt succeeded (or after logout swept the tier) wrote
the entry back, resurrecting a key that could silently re-redeem the
still-active link after an owner removes the collaborator.

Invalidation now lives in retainedShareKeys.ts as generation counters: a
capture takes a write token, and clearing a workspace's entry (or the logout
sweep) voids every token taken before it. The per-attempt flag is deleted --
its scope was the defect.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

* Bugfix: Clear a retained share key as soon as the keyed open succeeds.

The server confirms a share-key redemption inside open(), before the client
holds the capability -- but retention was discarded only after
subscribeToMetadata resolved. That call has real post-open failure modes for
exactly the keyed audience (the non-owner whoami round trip, a WS drop), and
every error path keeps the key by design, so a confirmed-then-failed
subscribe left all three retry paths (retry button, reconnect stub swap,
remount) armed with a live key -- and a re-redemption after an owner removal
silently re-grants access, since links are multi-redeemable and owner removal
wipes edges but not the link.

Keyed opens now await the open promise (one extra round trip, keyed opens
only -- the pipelined RpcPromise stays usable as the stub) and discard both
retention tiers the moment success is knowable; an open failure rejects there
and keeps retention, matching the server's reverted redemption. The tail
clear stays for the keyless corner where a retained entry existed but was
not attached (the identity-unknown path). The denial tests now model the
denial where it really lands -- openGadget's promise rejecting -- rather
than as a subscribeToMetadata throw.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

* Bugfix: Don't let a superseded keyed open clear a newer attempt's retained key.

The finding-7 fix awaited the keyed open and then unconditionally discarded
both retention tiers -- with no cancelled check, unlike every other
side-effect site in the hook. The await can park across the attempt's
cancellation, and a superseded attempt no longer owns the retention state: a
newer attempt may have captured its own key -- possibly another user's, on a
swapped stub -- into the very ref and sessionStorage entry the late clear
wipes, and clearRetainedShareKey's write-token bump also permanently voids
that attempt's still-in-flight identity stamp, so its failed open dead-ends
unretryable.

Bail before the clears when cancelled, matching the hook's invariant
everywhere else. Skipping the clear loses nothing: replaying the superseded
attempt's confirmed key later is a server-side no-op (a confirmed edge skips
redemption), and whichever attempt next succeeds clears retention itself.
The stub was assigned before the await, so the cleanup already disposed it
and a plain return is correct.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

* Bugfix: Let a cancelled keyed open clear its own retained share key.

The round-2 fix's blanket cancelled-bail before the post-open clears
discarded positive knowledge: reaching that line means the open
*resolved*, i.e. the server durably confirmed the redemption (nothing in
disposal reverts it). A confirmed-then-cancelled attempt (unmount, stub
swap, retry) left the identity-stamped sessionStorage entry behind --
the stamp is deliberately not cancelled-gated -- and every replay path
later re-redeemed the still-live link, silently restoring access after
an owner removal.

Clearing is now attempt-owned: clearRetainedShareKey takes an `onlyKey`
and no-ops (no removal, no generation bump) when the stored entry
carries a different key, so a newer capture's retention and in-flight
stamp survive -- which is what keeps the round-2 superseded-attempt test
passing unchanged -- while a matching or absent entry is cleared and its
pending stamp voided even after cancellation. The absent-entry bump is
deliberate fail-toward-security; its residual (voiding a concurrent
newer attempt's in-flight stamp) is documented with the recovery being
a re-click of the invite link.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

* Bugfix: Void a cleared share key's own pending stamp without touching other keys'.

The round-3 attempt-owned clear (clearRetainedShareKey with onlyKey) returned
without any invalidation when a different key occupied the entry -- so a
superseded attempt A whose open the server confirmed, but whose identity
stamp was still parked in whoami(), never voided that stamp: it landed late,
overwrote the newer attempt B's entry with A's *confirmed* key, and a later
mount replayed it -- re-redeeming the still-live link after an owner removal.
The generations were per-workspace, so A's stamp could not be voided without
also voiding B's.

Add a per-(workspace, key) generation tier: beginRetainedShareKeyWrite now
records the key it will stamp, commitRetainedShareKeyWrite checks all three
tiers, and an onlyKey clear always bumps exactly its own key's generation --
voiding the calling attempt's stamp even when a different key occupies the
entry -- while removing the entry only when it is absent or matching, and
leaving the workspace generation alone. That last part also retires round 3's
documented absent-entry residual: an attempt-owned clear can no longer void a
concurrent newer attempt's in-flight stamp.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

* Bugfix: Bail a cancelled attempt before it mutates newer retention.

In the retained-storage path, a cancelled attempt parked in whoami() resumed
and mutated retention before the pre-open cancelled check: the identity-match
branch re-armed the in-memory ref over a newer attempt's capture, and the
mismatch branch called the unscoped clearRetainedShareKey(id) -- sweeping a
newer attempt's entry and, via the workspace-generation bump, permanently
voiding its in-flight identity stamp.

Bail immediately after the identity resolves: a cancelled attempt no longer
owns retention, so it must neither re-arm the ref nor judge an entry that may
have been replaced while it was parked. And scope the identity-mismatch sweep
to the key this branch actually read and judged (clearRetainedShareKey(id,
retained.key), from the previous commit's key-scoped clears), so a newer
capture's different-key entry and stamp survive even if the branch is ever
reached with stale data.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

* Bugfix: Scope attempt-owned share-key clears by capture, not raw key.

Attempt-owned clears (the post-open success clear and the identity-
mismatch sweep) identified their retention by (workspaceId, raw key), so
two captures of the *same* invite key collided: after a same-tab user
switch, user A's disposed open of key K resolving late would remove user
B's freshly captured entry for the same K and permanently void B's
in-flight identity stamp (the per-(workspace, key) write generation was
shared), dead-ending B's retry on the access-denied page. An
availability bug only -- clearing is the fail-safe direction.

Each fragment capture now gets a unique captureId, stored in the entry,
the in-memory ref, and the write token. Attempt-owned clears bump that
capture's own generation and remove the entry only when it carries the
same captureId, so a same-key successor capture survives both the
removal and the stamp-voiding. Workspace-scoped and global clears are
unchanged.

The stored entry shape gains a required captureId with no migration:
the v2 format exists only on this branch, so entries without one simply
read as absent.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

* Mitigate duplicated-tab replay of a retained share key.

Browsers copy sessionStorage into a duplicated tab, so a retained
share-key entry cleared in the original tab lives on in the copy: after
a successful redemption and a later collaborator removal, the
duplicate's revocation-restart reconnect would silently re-redeem the
still-live link, undoing the removal.

Two frontend mitigations bound this without touching the kernel API
surface. Entries now expire 15 minutes after their identity stamp is
written (the legitimate failed-open retry/reload fits well inside that;
a copy replaying after a later removal does not), and clears propagate
across same-origin tabs over a BroadcastChannel -- capture-scoped
clears, which a duplicate's copied entry answers to because it shares
the original's captureId, and the logout sweep, since tabs share the
login session. Workspace-scoped clears name no capture and deliberately
stay local, so an independent sibling capture still legitimately
retrying is never blanket-cleared.

Documented residual: a duplicate discarded or unloaded at broadcast
time that reactivates within the TTL can still replay once. The link
itself stays multi-use server-side (docs/sharing.md already carries the
matching manual re-redeem residual); a single-use server-side retry
capability would close both and remains a possible kernel-side
follow-up.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

* Restate the retained-share-key rationale for one-step redemption.

Comments and the plan doc still described two-phase redemption: a failed
open reverting the redemption server-side, so a retry had to re-send the
key, and success "confirming" it. Under one-step redemption (#340) the
edge is real the moment the server redeems, so a failure after that
point retries keylessly and nothing is ever reverted or confirmed.

The mechanism is unchanged and still earns its keep: an open can fail
*before* the redemption lands (a transport failure, a server throw ahead
of the redemption, an attempt superseded before issuing), and the client
cannot distinguish that from a post-redemption failure, so it retains
the key on every failure -- replaying a key whose edge already exists is
a server-side no-op. Prose only; no behavior change.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

* Bugfix: Supersede an older capture's pending share-key stamp on a newer capture.

Write tokens tracked clears but not newer captures, so two captures for
one workspace with identity stamps in flight raced on the entry: when
the older capture's whoami() resolved last, its stamp overwrote the
newer capture's entry. Nothing had cleared, so no generation moved --
the older key (spent or not) became what a reload replayed under the
newer capture's session.

Starting a capture now bumps the workspace generation before taking its
token, so every older pending stamp for that workspace fails its
workspace check at commit time. The newest capture owns the slot
outright, whether or not the older attempt ever succeeded or cleared.
Other workspaces' pending stamps are untouched, and the per-capture tier
keeps its job: it is still what lets the newest capture's own success
clear void its stamp without voiding a later capture's.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

* Bugfix: Let a broadcast share-key clear reach a live duplicate's in-memory key.

The cross-tab clear broadcast swept a duplicated tab's sessionStorage
copy but not the two other places the same tab could still hold the
key: the hook's retainedShareKeyRef, re-armed from the copied entry
(same captureId), and a local the reload path had read the entry into
before parking in its identity await. "Try again" on the same stub
replayed the ref, and the resumed await attached the stale local -- so a
live duplicate re-redeemed the cleared key exactly as if nothing had
been broadcast.

Clears now notify subscribers (subscribeToRetainedShareKeyClears):
capture-scoped clears and the logout sweep, local and received alike,
with a capture-scoped clear reported whether or not a stored entry
matched, since the ref is a separate tier. Workspace-scoped clears stay
unreported: they name no capture, are never broadcast, and the hook
drops its own ref before issuing them. A listener's throw cannot break
the clear. The hook subscribes and drops its ref on a clear naming its
capture (or on the sweep), leaving a newer local capture with a
different id untouched; and the reload path re-reads storage after the
identity await, proceeding keylessly when the entry it judged has since
been swept (sibling broadcast, logout, or TTL).

The residual shrinks to what the module header already documents: only
a duplicate unloaded at broadcast time that reactivates within the TTL
can still replay once.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

* Bugfix: Remove an older capture's stored share-key entry when a newer capture begins.

Starting a capture superseded the older capture's *pending* stamp
(workspace generation bump) but left its already-committed entry in
storage until the new capture's own stamp landed. For one identity
round trip the slot held the older key, so a reload inside that window
replayed the older capture instead of the one just taken -- "the newest
capture owns the slot" was true of the stamp but not of the entry.

beginRetainedShareKeyWrite now also removes the workspace's stored
entry, after bumping the generation and before returning the token, so
the slot is empty rather than stale until the new stamp lands. The
removal is local only: a sibling tab's entry under the same workspace
is its own capture, or a duplicate's copy of an older one that the
older capture's own success clear reaches. Other workspaces' entries
and pending stamps, and the per-capture tier, are untouched.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

* Bugfix: Broadcast the clear of a share-key entry skipped by an unresolvable identity.

The reload path leaves a retained entry in place when whoami() rejects
(identity unknown, so the key is neither attached nor discarded). If the
keyless open then succeeds, the follow-up clear at the success site was
workspace-scoped -- and that scope names no capture, so it is never
broadcast. A duplicated tab's copy of the same entry (same captureId),
and a live duplicate's re-armed in-memory ref, survived the original's
success and could replay the still-live link after an owner removal.
Narrow -- it needs whoami() to fail while openGadget on the same stub
succeeds -- but it left one success path outside the "broadcast reaches
every tier" claim.

The keyless success now reads whatever entry is still stored for the
workspace and clears it by capture id first, which is the scope that
reaches siblings, before issuing the workspace-scoped clear that voids
the workspace's in-flight stamps. The leftover can only be the entry
this attempt read but could not judge, or nothing: a newer local
capture would have cancelled this attempt before it got here. Only
duplicates of this tab share the capture id, so the broadcast cannot
touch an independent sibling's capture, and a wrongly cleared key costs
a re-click of the link.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

* Reword the restricted-data sharing warning to match role-scoped verification.

The share modal's banner told an owner that invitees "must be verified
to have access to the same data". The server's check is narrower:
verification is scoped to the recipient's role (build recipients are
checked against every account-requiring connection present, use
recipients only against connections a gadget binds or an enabled hook
feeds), and producers that were never bound, are unbound, or were
removed after the read fall outside it -- their persisted output is
visible to anyone who can open the workspace (docs/observers.md edge
case 4). The banner is the copy an owner reads while deciding to share,
so it now states the guarantee actually made, and the disclosure that
goes with it: invitees verify their own access to the connections the
workspace uses at their access level, and anything already saved is
visible to everyone who can open it. The "Recipient verification" panel
beneath it already lists those connections per level. Prose only; no
server change.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

* Bugfix: Broadcast the clear of a share-key entry displaced by a newer capture.

beginRetainedShareKeyWrite removed the displaced entry locally only, on the
reasoning that a duplicated tab's copy of it would be reached by the displaced
capture's own success clear. That only holds if the displaced open ever
succeeds somewhere. Once a newer capture displaces it in the original tab, the
original's attempt is gone, so the only remaining clear path for the displaced
capture is the duplicate redeeming it itself. A duplicate whose own attempt
failed transiently keeps the copy; the newer capture's success clear names the
newer capture and misses it; and a reconnect in the duplicate inside the TTL
replays the displaced key after an owner may have removed the collaborator.

Clear the displaced entry by its capture id instead, which broadcasts. Capture
ids are per-capture UUIDs, so the clear can only reach copies of this tab's
entry, never an independent sibling capture, and this tab's own hook ref
already holds the new capture before the begin runs, so the local notification
drops nothing. The bare removal stays behind it for an entry the reader rejects
(malformed or v1), which no capture-scoped clear can name.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

* Bugfix: Clear an unjudged retained share key when its keyless open confirms after cancellation.

The identity-unknown reload path leaves a stored entry it could neither
attach nor judge, and the keyless success at the end of the attempt is
what discards it. A cancellation landing while the metadata subscribe
was in flight skipped that clear, even though the subscribe resolving
proves the keyless open succeeded (the pipelined call would have
rejected otherwise), so the spent entry survived for a later reload to
replay against the still-live link after an owner removal. Mirror the
keyed path's confirm-after-cancel clear, scoped to the capture this
attempt actually read so a newer attempt's entry and in-flight stamp
survive.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

* Bugfix: Leave another workspace's in-memory share key alone on a keyless success.

By the time a keyless open succeeds, any in-memory ref for that
workspace is already gone (dropped on a foreign stub, cleared by the
keyed success, or superseded by a newer capture cancelling the
attempt), so the unconditional ref clear could only ever drop a
different workspace's retention: capture a key for A, have A's open
fail transiently, open B keylessly, and A's same-stub retry lost its
key. Nothing about B's success says anything about A's key, and every
other ref clear in the hook is already scoped by id. Guard this one too.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

* Trim the retained-share-key comments.

Comments only. The rationale blocks in retainedShareKeys.ts and
useWorkspaceOpen.ts had grown by accretion over the bugfix series and
repeated themselves across sites; each is cut to what a reader needs at
that site, with the model and residuals stated once in the module and
ref headers.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

* Remove client-side share-key retention.

Reverts the retained-share-key series to the pre-series state: the
`#share=` fragment is stripped on open and sent once, and a first open
that fails before the server redeems the key is retried keylessly, so
the user re-clicks the invite link. Retaining the key across retries,
reconnects and reloads reopened replay-after-removal and cross-user
paths that took identity stamps, generation tokens, a TTL and cross-tab
broadcasts to close, and still left residuals (a mangled fragment stuck
until the TTL, a lost role upgrade), all to save one link re-click.

Restores useWorkspaceOpen.ts, its test, useAuth.ts, its test and
docs/sharing.md to their base versions, deletes retainedShareKeys.ts
and its test, and drops the retention items from the sharing plan.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

gatekeeper Changes to a gatekeeper integration kernel Changes to the Workshop kernel workshop/frontend Changes to the Workshop frontend workshop/shared Changes to shared Workshop APIs

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants